Shared AutoReport class for Temperature, Cardreader by thinkyhead · Pull Request #20913 · MarlinFirmware/Marlin

thinkyhead · 2021-01-29T01:31:16Z

Auto-report is a common paradigm in Marlin, so encapsulate this behavior as a simple template class.
Also apply macros like SEC_TO_MS where contextually-appropriate.

lightmaster · 2021-01-29T19:24:33Z

This one has broken Temp reporting for me. I rolled back to the commit before this one, and was able to get temps in Octoprint again.

)" This reverts commit 9d0e64a.

X-Ryl669 · 2021-01-30T14:58:15Z

Marlin/src/sd/cardreader.h

+    // SD Auto Reporting
+    //
+    #if HAS_MULTI_SERIAL
+      static serial_index_t auto_report_port;


This is wrong as a static variable is not a constexpr so it can't be used as a template parameter.

X-Ryl669 · 2021-01-30T15:02:44Z

Marlin/src/libs/autoreport.h

+    const millis_t ms = millis();
+    if (ELAPSED(ms, next_report_ms)) {
+      next_report_ms = ms + SEC_TO_MS(report_interval);
+      PORT_REDIRECT(AR_PORT_INDEX);


Beware, since the serial hooking code, PORT_REDIRECT expect a mask, not an index, you should use PORT_REDIRECT(SERIAL_MASK(AR_PORT_INDEX)) here instead.
There is no need for template parameter here, the mask is a runtime variable, it can be changed anytime!

X-Ryl669 · 2021-01-30T15:04:52Z

Marlin/src/sd/cardreader.h

+    #endif
+    class AutoReportSD : public AutoReporter<auto_report_port> { void auto_report(); };
+    static AutoReportSD auto_reporter;
  #endif


I think, there is no need for a template AutoReporter here, just set the serial_index_t as a member of the class since it's only used for PORT_REDIRECT which is a runtime check, and you can also remove the #if HAS_MULTI_SERIAL condition here so it'll be simpler.

X-Ryl669 · 2021-01-30T15:10:26Z

Marlin/src/module/temperature.h

-          auto_report_temp_interval = v;
-          next_temp_report_ms = millis() + 1000UL * v;
-        }
+        class AutoReportTemp : public AutoReporter<SERIAL_ALL> { void auto_report(); };


auto_report is not virtual so this is never called when manipulating the base class.

If you want to avoid the cost of a virtual table (since the instance is known at compile time), you can use CRTP here:

template <class Child> struct AutoReporter { [...] inline void auto_report() { static_cast<Child*>(this)->report(); } // Call the child's version. Does not need to be called auto_report [...] }; // You need a default implementation that does not report. struct NoAutoReport : public AutoReporter<NoAutoReport> { void report() {} }; // Then subclass like this: struct AutoReportTemp : public AutoReporter<AutoReportTemp> { void report() { ... } };

But this means you'll have to typedef the actual class to use since the type will change on each platform:

// In some common header, select the type to use based on compile time information: #ifdef HAS_MULTI_SERIAL typedef AutoReportMulti AutoReport; #else if ... typedef NoAutoReport AutoReport; [...] #endif // Ok, let's declare the reporter instance extern AutoReport auto_reporter;

X-Ryl669 · 2021-01-30T15:20:05Z

@lightmaster : Can you replace the line 33 from autoreport.h from:

 inline void auto_report() { }

to

virtual void auto_report() {}

It should fix it.

benlye · 2021-01-30T16:34:31Z

@lightmaster : Can you replace the line 33 from autoreport.h from:
 inline void auto_report() { }
to
virtual void auto_report() {}
It should fix it.

Confirmed that this change fixes temperature auto-reporting for me. (BTT SKR 1.4 Turbo, if it matters.)

Mezabaru · 2021-01-30T18:24:13Z

@lightmaster : Can you replace the line 33 from autoreport.h from:
 inline void auto_report() { }
to
virtual void auto_report() {}
It should fix it.
Confirmed that this change fixes temperature auto-reporting for me. (BTT SKR 1.4 Turbo, if it matters.)

Worked for me, ender 3 pro with skr mini E3

Guilouz · 2021-01-31T02:34:45Z

When I enable #define AUTO_REPORT_SD_STATUS, I can't compile, error :

When disable it it's ok

X-Ryl669 · 2021-01-31T07:11:52Z

@Guilouz Yes, it's known. It'll be fixed soon.

X-Ryl669 · 2021-01-31T23:28:08Z

@Guilouz Can you try #20959 (it should fix your build issue) and report ?

Guilouz · 2021-01-31T23:48:50Z

@Guilouz Can you try #20959 (it should fix your build issue) and report ?

Confirmed, it's woking !

X-Ryl669 · 2021-01-31T23:59:06Z

Thanks!

Guilouz · 2021-02-01T00:01:39Z

@X-Ryl669

But it's failed when I try to disable #define SDSUPPORT

Working with stable build 2.7.0.2 not with bugfix.

X-Ryl669 · 2021-02-01T00:04:20Z

Seems like it related #20846, that's another bug. Can you open a new issue ?

Guilouz · 2021-02-01T00:08:14Z

#20846

Yes I can.

…mware#20913)"" This reverts commit f301646.

…mware#20913)"" This reverts commit ae0eeb2.

Apply SEC_TO_MS macro

a66efa5

thinkyhead added the PR: Coding Standards label Jan 29, 2021

Shared AutoReport class for Temperature, Cardreader

3909cea

thinkyhead force-pushed the bf2_time_and_autoreport_PR branch from 58feb92 to 3909cea Compare January 29, 2021 01:41

thinkyhead merged commit 9d0e64a into MarlinFirmware:bugfix-2.0.x Jan 29, 2021

thinkyhead deleted the bf2_time_and_autoreport_PR branch January 29, 2021 02:40

lightmaster mentioned this pull request Jan 29, 2021

[BUG] Temperature no longer reported to OctoPrint #20921

Closed

mluckau added a commit to mluckau/MarlinTronxyX5SA-Pro that referenced this pull request Jan 29, 2021

Revert "AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913

f301646

)" This reverts commit 9d0e64a.

soularus added a commit to soularus/Marlin that referenced this pull request Jan 30, 2021

Revert "AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913

ae0eeb2

)" This reverts commit 9d0e64a.

X-Ryl669 reviewed Jan 30, 2021

View reviewed changes

GMagician mentioned this pull request Jan 30, 2021

Refactor serial class with templates #20783

Merged

X-Ryl669 pushed a commit to X-Ryl669/MarlinForMKS that referenced this pull request Jan 31, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

3033e8f

X-Ryl669 mentioned this pull request Jan 31, 2021

Fix AutoReporter not building and not working #20959

Merged

mluckau added a commit to mluckau/MarlinTronxyX5SA-Pro that referenced this pull request Feb 1, 2021

Revert "Revert "AutoReport class (Temperature, Cardreader) (MarlinFir…

e2e10f5

…mware#20913)"" This reverts commit f301646.

Jyers pushed a commit to Jyers/Marlin that referenced this pull request Feb 3, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

9e400d5

soularus added a commit to soularus/Marlin that referenced this pull request Feb 4, 2021

Revert "Revert "AutoReport class (Temperature, Cardreader) (MarlinFir…

c953402

…mware#20913)"" This reverts commit ae0eeb2.

kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

54bc1cb

zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

bb8112d

W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

ed0f2f4

thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

c7e581f

thinkyhead added a commit that referenced this pull request Apr 30, 2021

AutoReport class (Temperature, Cardreader) (#20913)

4b9f2f1

thinkyhead mentioned this pull request Dec 25, 2021

Refactor HAL as singleton #23295

Merged

Uh oh!

Conversation

thinkyhead commented Jan 29, 2021

Uh oh!

lightmaster commented Jan 29, 2021

Uh oh!

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

Uh oh!

X-Ryl669 commented Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benlye commented Jan 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mezabaru commented Jan 30, 2021

Uh oh!

Guilouz commented Jan 31, 2021

Uh oh!

X-Ryl669 commented Jan 31, 2021

Uh oh!

X-Ryl669 commented Jan 31, 2021

Uh oh!

Guilouz commented Jan 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

X-Ryl669 commented Jan 31, 2021

Uh oh!

Guilouz commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

X-Ryl669 commented Feb 1, 2021

Uh oh!

Guilouz commented Feb 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

X-Ryl669 commented Jan 30, 2021 •

edited

Loading

benlye commented Jan 30, 2021 •

edited

Loading

Guilouz commented Jan 31, 2021 •

edited

Loading

Guilouz commented Feb 1, 2021 •

edited

Loading